Skip to content

refactor: use typed errors in eth trace#6829

Merged
akaladarshi merged 3 commits intomainfrom
akaladarshi/typed-eth-trace-errors
Apr 1, 2026
Merged

refactor: use typed errors in eth trace#6829
akaladarshi merged 3 commits intomainfrom
akaladarshi/typed-eth-trace-errors

Conversation

@akaladarshi
Copy link
Copy Markdown
Collaborator

@akaladarshi akaladarshi commented Mar 31, 2026

Summary of changes

Changes introduced in this pull request:

  • Use typed errors in Eth trace

Reference issue to close (if applicable)

Closes #6784

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

Outside contributions

  • I have read and agree to the CONTRIBUTING document.
  • I have read and agree to the AI Policy document. I understand that failure to comply with the guidelines will lead to rejection of the pull request.

Summary by CodeRabbit

  • Refactor
    • Modernized trace error handling by introducing strongly-typed error structures, replacing untyped string-based errors with a categorized system that distinguishes between reverted transactions, out-of-gas conditions, and various VM-level failures for improved reliability and precision in trace operations.

@akaladarshi akaladarshi added the RPC requires calibnet RPC checks to run on CI label Mar 31, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 31, 2026

Walkthrough

This PR refactors error handling in the EthTrace struct from untyped Option<String> to a typed TraceError enum. Changes include introducing the TraceError enum with specific error variants, removing Parity error string constants, implementing custom serialization/deserialization logic for error round-tripping, and updating error mapping logic in the trace code.

Changes

Cohort / File(s) Summary
Error Type Definition
src/rpc/methods/eth/trace/types.rs
Introduced public TraceError enum with 11 variants (Reverted, OutOfGas, InvalidInstruction, UndefinedInstruction, StackUnderflow, StackOverflow, IllegalMemoryAccess, BadJumpDest, SelfDestructFailed, VmError(u32), ActorError(u32)). Implemented custom Serialize/Deserialize with specific string literals and parsing of "vm error: ..." / "actor error: ..." forms. Updated EthTrace::error field from Option<String> to Option<TraceError>. Modified is_reverted() and to_geth_error() methods for new type. Added comprehensive unit tests for JSON round-tripping and error conversions.
Error Mapping Logic
src/rpc/methods/eth/trace/parity.rs
Replaced error mapping function to return Option<TraceError> instead of Option<String>. Added exit-code-to-variant mappings: SYS_OUT_OF_GASTraceError::OutOfGas, system codes → TraceError::VmError(code), and actor/EVM-specific codes → dedicated variants. Removed exported Parity error string constants (PARITY_*). Updated imports to include TraceError.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

Suggested reviewers

  • hanabi1224
  • LesnyRumcajs
  • sudo-shashank
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'refactor: use typed errors in eth trace' clearly and specifically summarizes the main change - converting untyped string errors to typed error enums in Ethereum trace handling.
Linked Issues check ✅ Passed The PR successfully implements all objectives from issue #6784: replaces Option with a strongly-typed TraceError enum, reduces heap allocations via typed representation, and uses custom serialization instead of thiserror crate.
Out of Scope Changes check ✅ Passed All changes are directly related to the objective of implementing typed errors for EthTrace::error; no unrelated modifications are present in the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch akaladarshi/typed-eth-trace-errors
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch akaladarshi/typed-eth-trace-errors

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/rpc/methods/eth/trace/types.rs (1)

1319-1323: ⚠️ Potential issue | 🔴 Critical

Critical: Test helper uses incompatible string conversion.

The error field type changed from Option<String> to Option<TraceError>, but this test helper still uses "ErrForbidden".into(). Since TraceError doesn't implement From<&str> or From<String>, this code won't compile.

🐛 Proposed fix
             error: if result_address.is_none() {
-                Some("ErrForbidden".into())
+                Some(TraceError::from_string("ErrForbidden"))
             } else {
                 None
             },

Or use a more specific variant if appropriate:

             error: if result_address.is_none() {
-                Some("ErrForbidden".into())
+                Some(TraceError::ActorError(19)) // ErrForbidden exit code
             } else {
                 None
             },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/rpc/methods/eth/trace/types.rs` around lines 1319 - 1323, The test helper
is assigning a String into the new error field type Option<TraceError> (using
Some("ErrForbidden".into())), which doesn't compile; replace that String
conversion with a TraceError value (for example Some(TraceError::ErrForbidden)
or, if the enum provides a catch‑all,
Some(TraceError::Other("ErrForbidden".into()))), and update the match site
around result_address to construct the appropriate TraceError variant; ensure
TraceError is in scope (use the TraceError enum name used in this file) so the
test helper returns Option<TraceError> instead of Option<String>.
🧹 Nitpick comments (1)
src/rpc/methods/eth/trace/types.rs (1)

74-84: Consider adding a dedicated Unknown variant for unrecognized error strings.

The current fallback to ActorError(0) silently loses information when an unrecognized error string is encountered. While this is acceptable for round-tripping internally-produced data, a dedicated Unknown(String) variant would preserve the original error text and make debugging easier if unexpected strings appear.

💡 Optional: Add Unknown variant
 pub enum TraceError {
     // ... existing variants ...
     /// Actor-level error (catch-all for unrecognised exit codes).
     #[error("actor error: {}", ExitCode::from(*.0))]
     ActorError(u32),
+    /// Unknown error string that couldn't be parsed.
+    #[error("{0}")]
+    Unknown(String),
 }

Then in from_string:

                 } else {
-                    Self::ActorError(0)
+                    Self::Unknown(other.to_string())
                 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/rpc/methods/eth/trace/types.rs` around lines 74 - 84, The code currently
maps unrecognized error strings to ActorError(0); add a new enum variant
Unknown(String) to the enum in types.rs and update the from_string function to
return Self::Unknown(other.to_string()) in the final else branch (replacing
Self::ActorError(0)). Ensure references to the enum (e.g., in from_string, any
display/serialize helpers, and places that match on VmError/ActorError) are
updated to handle the Unknown(String) variant where appropriate so the original
error text is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/rpc/methods/eth/trace/types.rs`:
- Around line 1319-1323: The test helper is assigning a String into the new
error field type Option<TraceError> (using Some("ErrForbidden".into())), which
doesn't compile; replace that String conversion with a TraceError value (for
example Some(TraceError::ErrForbidden) or, if the enum provides a catch‑all,
Some(TraceError::Other("ErrForbidden".into()))), and update the match site
around result_address to construct the appropriate TraceError variant; ensure
TraceError is in scope (use the TraceError enum name used in this file) so the
test helper returns Option<TraceError> instead of Option<String>.

---

Nitpick comments:
In `@src/rpc/methods/eth/trace/types.rs`:
- Around line 74-84: The code currently maps unrecognized error strings to
ActorError(0); add a new enum variant Unknown(String) to the enum in types.rs
and update the from_string function to return Self::Unknown(other.to_string())
in the final else branch (replacing Self::ActorError(0)). Ensure references to
the enum (e.g., in from_string, any display/serialize helpers, and places that
match on VmError/ActorError) are updated to handle the Unknown(String) variant
where appropriate so the original error text is preserved.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: c4f73856-c22a-404e-88d6-4f56621ae0dc

📥 Commits

Reviewing files that changed from the base of the PR and between 0b7f2b4 and 9029fe3.

📒 Files selected for processing (2)
  • src/rpc/methods/eth/trace/parity.rs
  • src/rpc/methods/eth/trace/types.rs

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 31, 2026

Codecov Report

❌ Patch coverage is 83.33333% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.89%. Comparing base (feeb998) to head (9b23986).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/rpc/methods/eth/trace/parity.rs 16.66% 10 Missing ⚠️
src/rpc/methods/eth/trace/types.rs 96.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/rpc/methods/eth/trace/types.rs 93.62% <96.66%> (+0.35%) ⬆️
src/rpc/methods/eth/trace/parity.rs 65.21% <16.66%> (ø)

... and 6 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update feeb998...9b23986. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@akaladarshi akaladarshi requested a review from a team as a code owner March 31, 2026 15:18
@akaladarshi akaladarshi requested review from LesnyRumcajs and hanabi1224 and removed request for a team March 31, 2026 15:18
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/rpc/methods/eth/trace/types.rs (1)

49-52: Avoid the extra allocation in Serialize.

serialize_str(&self.to_string()) still allocates a String for every serialized error. serializer.collect_str(self) writes the Display impl directly and keeps the allocation win from the typed representation.

♻️ Proposed fix
 impl Serialize for TraceError {
     fn serialize<S: serde::Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
-        serializer.serialize_str(&self.to_string())
+        serializer.collect_str(self)
     }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/rpc/methods/eth/trace/types.rs` around lines 49 - 52, The Serialize impl
for TraceError currently allocates by calling self.to_string(); change the
serializer call inside impl Serialize for TraceError (the serialize method) to
use serializer.collect_str(self) so the Display impl is written directly without
creating an intermediate String; locate the Serialize impl for TraceError and
replace the serialize_str(&self.to_string()) usage accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/rpc/methods/eth/trace/types.rs`:
- Around line 63-84: The parser should not coerce unrecognized or malformed
error strings to ActorError(0)/VmError(0); update the enum used by from_string
to either add an Unknown(String) variant (preferred) or make from_string
fallible (Result/Option), then change from_string to return
Unknown(original_input) instead of Self::ActorError(0)/Self::VmError(0) when no
known pattern matches and to propagate parse_exit_code_display failures instead
of defaulting to 0; also add a doc comment to the public from_string explaining
the accepted wire formats and behaviors.

---

Nitpick comments:
In `@src/rpc/methods/eth/trace/types.rs`:
- Around line 49-52: The Serialize impl for TraceError currently allocates by
calling self.to_string(); change the serializer call inside impl Serialize for
TraceError (the serialize method) to use serializer.collect_str(self) so the
Display impl is written directly without creating an intermediate String; locate
the Serialize impl for TraceError and replace the
serialize_str(&self.to_string()) usage accordingly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: d7ab684b-20f3-4672-b311-c785b1d205ee

📥 Commits

Reviewing files that changed from the base of the PR and between 0b7f2b4 and 9ce2e88.

📒 Files selected for processing (2)
  • src/rpc/methods/eth/trace/parity.rs
  • src/rpc/methods/eth/trace/types.rs

Comment thread src/rpc/methods/eth/trace/types.rs
@LesnyRumcajs
Copy link
Copy Markdown
Member

no green checkmark, no review!

@akaladarshi akaladarshi enabled auto-merge April 1, 2026 08:45
@akaladarshi akaladarshi added this pull request to the merge queue Apr 1, 2026
Merged via the queue into main with commit ad8b49d Apr 1, 2026
60 of 62 checks passed
@akaladarshi akaladarshi deleted the akaladarshi/typed-eth-trace-errors branch April 1, 2026 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RPC requires calibnet RPC checks to run on CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor: Use typed errors for EthTrace::error instead of Option<String>

3 participants